Support for recovery hash in Shutter DK #2100
Conversation
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughImplements juror recovery for Shutter dispute kits (store per‑vote recovery commits and allow juror reveals without justification), refactors Classic dispute‑kit vote lookups and expected‑hash access, updates hashVote mutability, adds tests, and updates build/config and CI workflow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant DK as DisputeKitShutter
participant Storage as OnChain
rect rgb(250,250,255)
note over Caller,DK: Commit phase
Caller->>DK: castCommitShutter(coreDisputeID, voteIDs, commit, recoveryCommit, ...)
DK->>Storage: store votes[local].commit
DK->>Storage: store recoveryCommitments[local]
DK-->>Caller: emit CommitCastShutter(..., recoveryCommit)
end
rect rgb(245,255,245)
note over Caller,DK: Reveal (castVoteShutter)
Caller->>DK: castVoteShutter(coreDisputeID, voteID, choice, salt, justification)
DK->>DK: callerIsJuror = (msg.sender == juror)
alt callerIsJuror == true
DK->>DK: actual = hashVote(choice,salt,justification) %% returns hash(choice+salt)
DK->>Storage: expected = recoveryCommitments[local]
else callerIsJuror == false
DK->>DK: actual = hashVote(choice,salt,justification) %% returns hash(choice+salt+justifHash)
DK->>Storage: expected = votes[local].commit
end
DK->>DK: compare actual == expected → accept or revert
DK->>DK: reset callerIsJuror
DK-->>Caller: emit VoteCast / revert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
jaybuidl
added
Type: Feature🗿
Package: Contracts
jaybuidl
changed the title
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
117-133: Guard against empty _voteIDs to avoid OOB revert and preserve intended error semanticscastVoteShutter reads _voteIDs[0] before delegating to _castVote. If _voteIDs is empty, this will revert with an OOB panic instead of EmptyVoteIDs. Add an explicit length check first.
Apply this diff:
- Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; + if (_voteIDs.length == 0) revert EmptyVoteIDs(); + Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];Optionally, restore callerIsJuror after the call using a save/restore pattern to be future-proof against nested flows:
- callerIsJuror = juror == msg.sender; - // `_castVote()` ensures that all the `_voteIDs` do belong to `juror` - _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror); - callerIsJuror = false; + bool prev = callerIsJuror; + callerIsJuror = (juror == msg.sender); + // `_castVote()` ensures that all the `_voteIDs` do belong to `juror` + _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror); + callerIsJuror = prev;
🧹 Nitpick comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (3)
28-29: Using transient storage for caller role — correct for CancunTransient bool callerIsJuror is appropriate for request-scoped switching of hashing behavior and avoids persistent state. With both toolchains set to Cancun, this should work as intended.
Consider a brief NatSpec note near the declaration clarifying that it influences hashVote() behavior during castVoteShutter only.
103-115: Write recoveryCommitments only after ownership/period checks to avoid wasted gas on revertYou populate recoveryCommitments before calling _castCommit. Although a revert in _castCommit will roll back these writes, the SSTORE gas is still consumed in the failing path. Move the writes after _castCommit to avoid unnecessary gas burn on invalid calls.
Apply this diff:
- if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); - - uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; - } - - // `_castCommit()` ensures that the caller owns the vote - _castCommit(_coreDisputeID, _voteIDs, _commit); + if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); + + // `_castCommit()` ensures that the caller owns the vote and we are in commit period. + _castCommit(_coreDisputeID, _voteIDs, _commit); + + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; + }
150-159: Context-sensitive hashVote: confirm off-chain commit derivation matches client librarieshashVote now depends on callerIsJuror:
- juror path: keccak(abi.encodePacked(choice, salt))
- non-juror path: keccak(abi.encode(choice, salt, keccak256(bytes(justification))))
This is fine for on-chain verification, but it changes expectations for off-chain commitment generation. Please confirm that the Shutter client/wallet libraries compute:
- commit (with justification) as keccak(abi.encode(choice, salt, keccak256(bytes(justification))))
- recoveryCommit (without justification) as keccak(abi.encodePacked(choice, salt))
If prior versions used abi.encodePacked for the justification-including commit, this is an ABI-breaking change for commit preimages.
I can scan your SDKs/examples to ensure these formulas match. Want me to run a targeted repo search?
Consider adding a NatSpec note here stating the context-dependent behavior to prevent integrator confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
contracts/foundry.toml(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Pages changed - kleros-v2-university
🔇 Additional comments (10)
contracts/foundry.toml (1)
4-7: Aligning Cancun EVM target and raising optimizer runs — LGTMSetting evm_version = "cancun" and bumping optimizer_runs to 10000 aligns Foundry with the Solidity 0.8.30 toolchain and the Hardhat config in this PR. This is required for transient storage (EIP-1153) used in Shutter. No action needed.
If you want an extra safety check for deterministic bytecode across toolchains, run a quick cross-build compare in CI (Foundry vs Hardhat) on a small sample contract to catch accidental setting drift.
contracts/hardhat.config.ts (1)
31-36: Hardhat now targeting Cancun — consistent with FoundryAdding evmVersion: "cancun" keeps Hardhat in sync with Foundry and enables compilation of transient storage (used by DisputeKitShutter). Optimizer runs = 10000 also match Foundry. Looks good.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
317-332: Hidden-vote validation now routed through an overrideable expected-hash — solid hook pointSwitching to localDisputeID/localRoundID and comparing hashVote(...) against getExpectedVoteHash(...) cleanly decouples “how to compute/where to fetch expected commits” from the base flow. This enables Shutter’s dual-hash scheme without touching core logic. Nice separation of concerns.
491-494: hashVote mutability change (pure → view) to allow overrides — OKThe base implementation remains keccak(choice, salt). Documenting the unused justification parameter in the signature is helpful for overrides. No issues.
715-727: New internal hook getExpectedVoteHash — good extensibility pointProviding a virtual getter for the expected hash is the right way to support alternative commitment schemes (e.g., recovery commitments in Shutter). No concerns.
737-737: Minor arg name anonymization to reduce stack footprint — fineRenaming the Round parameter to an unused placeholder is harmless and helps with “stack too deep” pressure in downstream overrides.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (4)
21-23: New recoveryCommitments storage — acknowledge storage layout breakAdding recoveryCommitments is a storage-layout change versus previously deployed versions. Given the PR explicitly plans a full redeploy (not upgrade), this is acceptable. Ensure all deployment scripts and on-chain addresses reflect the redeploy plan to avoid proxy upgrades on old storage layouts.
Do you want a quick checklist PR comment for “full redeploy” steps (addresses, permit lists, court params, governor handoff, etc.)?
38-46: CommitCastShutter ABI change: _recoveryCommit addedEvent shape change is ABI-breaking as noted. Make sure indexation choices match consumer needs: you kept _commit indexed and left _recoveryCommit non-indexed (reasonable since topics are limited). Communicate this to indexers/SDKs.
If downstream infra filters on the old event signature, flag them to update to the new one.
167-176: getExpectedVoteHash override correctly switches between commit and recoveryCommitThis matches the intended flows:
- juror caller: validate against recoveryCommitments
- non-juror caller: validate against stored commit
Looks correct.
182-183: EmptyRecoveryCommit error — good explicit revertClear error improves debuggability at commit time.
…cted, causing stack too deep
jaybuidl
force-pushed
the
feat/shutter-recovery-hash
branch
from
f494977 to
bd589e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
764-764: Unused Round param kept for compat; consider removing on next breaking releaseSignature is cleaner now with the placeholder. Mark for removal in the next storage/ABI-breaking cycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
contracts/foundry.toml(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol
- contracts/foundry.toml
- contracts/hardhat.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
321-327: LGTM: switching to local IDs reduces repeated mappings and gasGood use of localDisputeID/localRoundID; simplifies indexing and saves reads.
742-754: Good extensibility: expected-hash hook for caller-aware DKsThis internal virtual hook cleanly supports Shutter DK (commit vs recoveryCommit). Please confirm Shutter DK overrides it to:
- return recoveryCommit when msg.sender == vote.account,
- otherwise return the original commit including justification.
492-498: hashVote mutability updated—confirm derived DKs and interfaces updated accordingly
Verify that DisputeKitShutter.sol and DisputeKitGatedShutter.sol (and any other overrides) declarehashVotewithview(notpure) and that no external interfaces still expect apuresignature.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
280-287: Prevent multi-commit overcount; make areVotesAllCast() reachable.totalCommitted increments even on re-commit, blocking hidden-vote completion.
Apply:
- for (uint256 i = 0; i < _voteIDs.length; i++) { - if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote(); - round.votes[_voteIDs[i]].commit = _commit; - } - round.totalCommitted += _voteIDs.length; + uint256 newlyCommitted; + for (uint256 i = 0; i < _voteIDs.length; i++) { + uint256 vId = _voteIDs[i]; + if (round.votes[vId].account != msg.sender) revert JurorHasToOwnTheVote(); + if (round.votes[vId].commit == bytes32(0)) { + unchecked { newlyCommitted++; } + } + round.votes[vId].commit = _commit; + } + round.totalCommitted += newlyCommitted;
♻️ Duplicate comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
328-336: Fail fast and only hash when hiddenVotes is true.Reorder to check VoteAlreadyCast first and avoid unnecessary hashing/storage reads; compute hash only when needed.
Apply:
- bytes32 actualVoteHash = hashVote(_choice, _salt, _justification); + bytes32 actualVoteHash; // Save the votes. for (uint256 i = 0; i < _voteIDs.length; i++) { if (round.votes[_voteIDs[i]].account != _juror) revert JurorHasToOwnTheVote(); - if (hiddenVotes && _getExpectedVoteHash(localDisputeID, localRoundID, _voteIDs[i]) != actualVoteHash) - revert HashDoesNotMatchHiddenVoteCommitment(); if (round.votes[_voteIDs[i]].voted) revert VoteAlreadyCast(); + if (hiddenVotes) { + if (actualVoteHash == bytes32(0)) { + actualVoteHash = hashVote(_choice, _salt, _justification); + } + if (_getExpectedVoteHash(localDisputeID, localRoundID, _voteIDs[i]) != actualVoteHash) { + revert HashDoesNotMatchHiddenVoteCommitment(); + } + }
🧹 Nitpick comments (3)
contracts/CHANGELOG.md (1)
35-39: Add explicit migration note: full redeploy required due to storage layout change.Make the breaking impact unmistakable in release notes.
Apply:
- **Breaking:** Add a storage variable `recoveryCommitments` to `DisputeKitShutter` ([#2100](https://github.com/kleros/kleros-v2/issues/2100)) + - **Breaking:** Add a storage variable `recoveryCommitments` to `DisputeKitShutter` (requires fresh deployment; not upgrade-safe with existing storage layouts) ([#2100](https://github.com/kleros/kleros-v2/issues/2100))contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
109-117: Avoid wasted SSTORE on failing commits: write recoveryCommitments after _castCommit.If _castCommit reverts (wrong owner/period), current code pays for SSTOREs that will be reverted. Do checks first, then store.
Apply:
- if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); - - uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; - } - - // `_castCommit()` ensures that the caller owns the vote + if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); + // `_castCommit()` ensures correct period/ownership and non-empty commit. _castCommit(_coreDisputeID, _voteIDs, _commit); + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + uint256 localRoundID = disputes[localDisputeID].rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; + } emit CommitCastShutter(_coreDisputeID, msg.sender, _commit, _recoveryCommit, _identity, _encryptedVote);
21-29: Transient flag usage: keep scope minimal and avoid external calls while set if possible.callerIsJuror gates hash/expected-hash. Since _castVote does external static calls to core, minimizing the window and always resetting is good practice (you already reset at the end). Consider wrapping set/call/reset in a small internal helper to reduce future footguns if logic changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
contracts/CHANGELOG.md(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
321-327: Local IDs refactor LGTM.Using localDisputeID/localRoundID improves clarity and aligns with Shutter override path.
495-497: hashVote mutability/signature change LGTM.View + placeholder justification enables DK-specific overrides without breaking Classic behavior.
742-754: _getExpectedVoteHash abstraction LGTM.Clean hook for Shutter to swap commit source.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
156-165: Confirm off-chain commit derivation matches encoding here.You use abi.encode(_choice, _salt, keccak256(bytes(_justification))) for the auto-reveal path and abi.encodePacked(_choice, _salt) for recovery. Ensure frontend/keeper compute exactly these to avoid reveal mismatches.
3-3: Compiler/EVM settings alignment.pragma ^0.8.28 with transient storage implies Cancun. Ensure hardhat/foundry evmVersion=cancun is enforced across builds and target chains support it.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
215-229: Incorrect length guard before mload — possible garbage readThe function reads at offsets 0x80 and 0xA0 (128 and 160). You need at least 5 words (5×32=160 data bytes after the 32-byte length header), i.e., total bytes length >= 192 to safely
mloadthe 5th word. The current< 160check is insufficient and may read uninitialized memory, leading to incorrect gating decisions.Apply this diff to harden the guard and comment:
- // Need at least 160 bytes to safely read the parameters - if (_extraData.length < 160) return (address(0), false, 0); + // Need at least 192 bytes (5 x 32-byte words) to safely read the parameters + if (_extraData.length < 192) return (address(0), false, 0);Optional: replace the assembly with safer decoding:
- assembly { - let packedTokenGateIsERC1155 := mload(add(_extraData, 0x80)) - tokenId := mload(add(_extraData, 0xA0)) - tokenGate := and(packedTokenGateIsERC1155, 0xffffffffffffffffffffffffffffffffffffffff) - isERC1155 := and(shr(160, packedTokenGateIsERC1155), 1) - } + ( , , , uint256 packed, uint256 _tokenId) = abi.decode(_extraData, (uint96, uint256, uint256, uint256, uint256)); + tokenGate = address(uint160(packed)); + isERC1155 = (packed >> 160) & 1 == 1; + tokenId = _tokenId;
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
172-181: Use consistent ABI encoding in both branches of hashVoteFunction comment says “ABI encoding”. For fixed-size types,
abi.encodeandabi.encodePackedproduce equivalent length, but using one consistently avoids confusion.Apply this diff:
- if (callerIsJuror) { - // Caller is the juror, hash without `_justification` to facilitate recovery. - return keccak256(abi.encodePacked(_choice, _salt)); + if (callerIsJuror) { + // Caller is the juror, hash without `_justification` to facilitate recovery. + return keccak256(abi.encode(_choice, _salt));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(6 hunks)
🔇 Additional comments (7)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (7)
33-36: Section header OKNo action needed.
37-39: New recoveryCommitments mapping — confirm upgrade pathPublic triple-keyed mapping looks fine. Given storage layout changes, ensure this contract is only used in fresh deployments (no proxy upgrade). If this replaces an existing deployed impl, please confirm you’ll redeploy per the PR note.
40-45: Transient flag requires Cancun EVM at compile+runtimeUsing
bool transient callerIsJuror;is good for tx-scoped state. Please confirm:
- Compiler supports
transientfor pragma ^0.8.24 in your toolchain.- Hardhat/Foundry evmVersion is set to “cancun” for this target so TSTORE/TLOAD are available on-chain/networks you deploy to.
54-54: Docstring addition is clearGood clarification that
_recoveryCommitexcludes justification.
61-61: Event extended with _recoveryCommitLooks good. Keeping it non-indexed is correct (already using 3 indexed fields).
187-203: _getExpectedVoteHash logic LGTMClean split: juror path uses recoveryCommit, others use on-chain commit. This pairs correctly with the transient flag.
254-260: Custom error addition OK
EmptyRecoveryCommit()is appropriate and used correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/test/arbitration/dispute-kit-shutter.ts (3)
181-204: Improve error handling in getVoteIDsForJurorThe function uses a try-catch to detect when there are no more votes, but this approach could mask other legitimate errors. Consider checking for a specific error or using a more explicit way to determine the number of votes.
const getVoteIDsForJuror = async (disputeId: number, juror: HardhatEthersSigner) => { const localDisputeId = await disputeKitShutter.coreDisputeIDToLocal(disputeId); const nbRounds = await disputeKitShutter.getNumberOfRounds(localDisputeId); const roundIndex = Number(nbRounds) - 1; // Get all votes for this round and filter by juror const voteIDs: bigint[] = []; - const maxVotes = 10; // Reasonable limit for testing + // Consider getting the actual number of votes from the contract if available + const maxVotes = 10; // Reasonable limit for testing for (let i = 0; i < maxVotes; i++) { try { const voteInfo = await disputeKitShutter.getVoteInfo(disputeId, roundIndex, i); if (voteInfo[0] === juror.address) { // account is at index 0 voteIDs.push(BigInt(i)); } - } catch { - // No more votes + } catch (error) { + // Check for specific revert reason if possible + // For now, assume any error means no more votes break; } } return voteIDs; };
625-636: Consider testing hashVote for juror contextThe test only verifies the hash computation for non-jurors. Consider adding a test that verifies the hash computation when called by a juror (recovery case) to ensure complete coverage of both paths.
Add a test case for juror-context hashing:
it("Should correctly compute hash for recovery flow", async () => { // Test that hashVote behaves differently when called by the juror // This would require setting up a dispute with a juror commitment first const recoveryHash = ethers.keccak256( ethers.AbiCoder.defaultAbiCoder().encode(["uint256", "uint256"], [choice, salt]) ); // Note: This test might need modification based on how hashVote determines caller context // You may need to verify this behavior through the castVoteShutter flow instead });
166-167: Incorrect court struct commentThe comment about the Court struct fields appears to be outdated or incorrect. The actual Court struct likely has a different structure than described in the comment.
- // Court struct: parent, hiddenVotes, children[], minStake, alpha, feeForJuror, jurorsForCourtJump, disabled, timesPerPeriod[] - // timesPerPeriod is a mapping, we need to check the actual structure + // Get timesPerPeriod values for the court
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
contracts/test/arbitration/dispute-kit-shutter.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/test/arbitration/dispute-kit-shutter.ts (1)
contracts/deploy/utils/index.ts (1)
PNK(40-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Mend Security Check
🔇 Additional comments (9)
contracts/test/arbitration/dispute-kit-shutter.ts (9)
96-109: Well-structured commitment generation logicThe
generateCommitmentsfunction correctly implements the dual-hash approach for recovery commitments as per the PR objectives. The recovery commitment excludes justification while the full commitment includes it, enabling the juror recovery flow.
212-234: Comprehensive test for recovery commitment storageExcellent test coverage for the core recovery commitment feature. The test properly verifies that the recovery commitment is stored correctly and emits the expected event with all parameters including the new
recoveryCommitfield.
267-285: Good validation of empty recovery commitment errorThe test correctly validates that an empty recovery commitment (ZeroHash) is rejected with the
EmptyRecoveryCommiterror, ensuring the integrity of the recovery mechanism.
459-492: Excellent recovery flow test coverageThe test thoroughly validates the juror recovery scenario where justification can be omitted or different. This directly addresses the PR objective of allowing jurors who remember their vote but not the exact justification to recover their committed vote.
494-519: Critical test for justification flexibility in recoveryThis test is crucial as it validates that jurors can provide a different justification during recovery, which is the primary objective of this PR. The test confirms that the recovery commitment (without justification) is used for validation when the juror reveals.
575-599: Important security test for non-juror reveal attemptsExcellent test validating that non-jurors (attackers) cannot exploit the recovery mechanism. The test ensures that non-jurors must still provide the exact justification matching the full commitment.
639-686: Comprehensive mixed reveal scenario testExcellent edge case testing that validates both normal and recovery reveals can coexist in the same dispute. This ensures the system correctly handles hybrid scenarios where Shutter works for some jurors but fails for others.
688-732: Good security validation for vote integrityThe test properly validates that while anyone can reveal votes with correct data (as intended for bot operation), they cannot alter or manipulate the actual vote choices. This maintains the cryptographic integrity of the voting system.
1-735: Overall excellent test coverage for recovery commitments featureThe test suite comprehensively covers the new recovery commitment functionality with appropriate positive and negative test cases. The tests validate:
- Dual commitment storage (full and recovery)
- Juror vs non-juror reveal behavior
- Security against manipulation
- Edge cases with mixed reveal scenarios
The implementation aligns well with the PR objectives of allowing juror recovery without exact justification matching while maintaining system integrity.
jaybuidl
force-pushed
the
feat/shutter-recovery-hash
branch
from
e405264 to
b490c96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
.github/workflows/contracts-testing.yml (6)
76-78: Remove Foundry install from Hardhat job (or align versions).
Foundry isn’t used in hardhat-tests; installing v1.3.1 here while using v1.4.0 in foundry-tests also introduces drift.Option A (remove):
- - name: Install Foundry - uses: foundry-rs/foundry-toolchain@de808b1eea699e761c404bda44ba8f21aba30b2c # v1.3.1Option B (if needed for Anvil, align to v1.4.0):
- uses: foundry-rs/foundry-toolchain@de808b1eea699e761c404bda44ba8f21aba30b2c # v1.3.1 + uses: foundry-rs/foundry-toolchain@82dee4ba654bd2146511f85f0d013af94670c4de # v1.4.0
61-72: Improve cache effectiveness for Yarn 4.
Yarn 4 primarily uses the project’s .yarn/cache. Consider caching it to reduce cold starts; keeping node_modules is fine if nodeLinker=node-modules.path: | - ~/.npm - **/node_modules + ~/.npm + **/node_modules + **/.yarn/cacheAlso applies to: 121-132
27-27: Add job timeouts to prevent hung runners.
Helps avoid stuck CI.hardhat-tests: - runs-on: ubuntu-latest + runs-on: ubuntu-latest + timeout-minutes: 45foundry-tests: - runs-on: ubuntu-latest + runs-on: ubuntu-latest + timeout-minutes: 45Also applies to: 87-87
1-22: Optional: add concurrency to auto-cancel superseded runs on PR updates.
Reduces CI queue time and cost.name: Contracts Testing @@ on: workflow_dispatch: merge_group: @@ pull_request: branches: - "*" + +concurrency: + group: contracts-tests-${{ github.workflow }}-${{ github.ref || github.run_id }} + cancel-in-progress: true
139-141: Run Foundry tests in offline mode under egress block.
If all deps and submodules are present, this hardens determinism and avoids unexpected network calls.- - name: Run Foundry tests - run: forge test + - name: Run Foundry tests + env: + FOUNDRY_OFFLINE: "true" + run: forge test
55-60: Minor: you already enable Yarn cache via setup-node; consider dropping separate node_modules cache if using PnP.
Only if nodeLinker=pnp. Otherwise keep as-is.Also applies to: 115-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/contracts-testing.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: foundry-tests
🔇 Additional comments (2)
.github/workflows/contracts-testing.yml (2)
19-21: Permissions are minimal and appropriate.
contents: read is sufficient for this CI. No objections.
23-31: Split Hardhat and Foundry jobs: good separation of concerns.
Clearer ownership and parallelism; nice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/contracts-testing.yml (1)
34-43: Add missing GitHub API/download endpoints to Harden Runner allowlist (same ask as earlier)Without these, checkout/submodules, action metadata, or release assets may intermittently fail under egress-policy: block. Apply to both jobs.
allowed-endpoints: > binaries.soliditylang.org:443 + api.github.com:443 classic.yarnpkg.com:443 + codeload.github.com:443 github.com:443 nightly.yarnpkg.com:443 nodejs.org:443 objects.githubusercontent.com:443 + raw.githubusercontent.com:443 registry.yarnpkg.com:443 registry.npmjs.org:443 54.185.253.63:443Also applies to: 91-100
🧹 Nitpick comments (4)
.github/workflows/contracts-testing.yml (4)
61-72: Fix dependency caching: cache Yarn Berry’s project cache, not node_modules/npmYarn v4 (Berry) doesn’t use ~/.npm, and caching node_modules is brittle. Cache .yarn/cache instead for correctness and faster restores. Apply in both jobs.
- - name: Cache node modules + - name: Cache Yarn cache uses: actions/cache@0c907a75c2c80ebcb7f088228285e798b750cf8f # v4.2.1 - env: - cache-name: cache-node-modules with: - path: | - ~/.npm - **/node_modules - key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }} - restore-keys: | - ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}- + path: ./.yarn/cache + key: ${{ runner.os }}-yarn-${{ secrets.CACHE_VERSION }}-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-yarn-${{ secrets.CACHE_VERSION }}-Optional: drop
cache: yarnin setup-node to avoid redundant caches.Also applies to: 118-129
73-75: Prefer immutable installs; avoidworkspace ... installEnsure reproducible installs and correct Berry usage. Recommend a top-level immutable install (and optionally focus the contracts workspace).
- - name: Install contracts dependencies - run: yarn workspace @kleros/kleros-v2-contracts install + - name: Install dependencies (immutable) + run: | + yarn install --immutable + yarn workspaces focus @kleros/kleros-v2-contracts --allAlso applies to: 130-132
136-139: Use working-directory instead of inlined cd for consistencyMinor style/maintainability improvement; mirrors the snapshot step.
- - name: Run Foundry tests - run: cd contracts && forge test - # working-directory: contracts + - name: Run Foundry tests + run: forge test + working-directory: contracts
43-43: Avoid allowlisting raw IPs; prefer domains
54.185.253.63:443is brittle (IP may change, unclear owner). Replace with the owning domain or justify necessity.Also applies to: 100-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/contracts-testing.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: hardhat-tests
- GitHub Check: foundry-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
.github/workflows/contracts-testing.yml (1)
23-27: Split Hardhat/Foundry jobs improves isolation and debuggability — LGTMClear separation with banner comments is helpful. No issues spotted in the job scaffolding.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/contracts-testing.yml (1)
34-43: Add missing GitHub API/download endpoints to Harden Runner allowlist (Hardhat job).These are common and were flagged before; without them, intermittent failures occur.
allowed-endpoints: > binaries.soliditylang.org:443 + api.github.com:443 classic.yarnpkg.com:443 + codeload.github.com:443 github.com:443 nightly.yarnpkg.com:443 nodejs.org:443 objects.githubusercontent.com:443 + raw.githubusercontent.com:443 registry.yarnpkg.com:443 registry.npmjs.org:443 54.185.253.63:443
🧹 Nitpick comments (4)
.github/workflows/contracts-testing.yml (4)
61-72: Tune Yarn cache paths; drop npm cache.This job uses Yarn v4—cache .yarn cache; ~/.npm is irrelevant.
with: path: | - ~/.npm - **/node_modules + **/.yarn/cache + **/node_modules key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }}
118-129: Mirror Yarn cache tuning in Foundry job.Same rationale as Hardhat job.
with: path: | - ~/.npm - **/node_modules + **/.yarn/cache + **/node_modules key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }}
34-43: Avoid raw IP allowlisting; use FQDNs.54.185.253.63:443 is brittle and non-descriptive. Prefer the canonical host it represents.
Also applies to: 92-101
26-43: Set job-level timeout to prevent hung runs.Helps avoid stuck CI when upstream registries stall.
hardhat-tests: - runs-on: ubuntu-latest + runs-on: ubuntu-latest + timeout-minutes: 30 ... foundry-tests: - runs-on: ubuntu-latest + runs-on: ubuntu-latest + timeout-minutes: 30Also applies to: 83-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/contracts-testing.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
.github/workflows/contracts-testing.yml (1)
140-146: Ensure forge is on PATH across steps.If keeping curl-based install, the previous fix (GITHUB_PATH) is required; otherwise forge won’t be found here. Verify before merging.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/contracts-testing.yml (3)
19-21: Enable cache writes (add actions: write).actions/cache won’t save without actions: write at workflow or job scope.
permissions: # added using https://github.com/step-security/secure-workflows contents: read + actions: write
34-43: Harden Runner allowlist: add common GitHub endpoints; avoid brittle IP.Add API/download hosts to prevent intermittent egress blocks; drop the hard-coded IP.
allowed-endpoints: > binaries.soliditylang.org:443 + api.github.com:443 classic.yarnpkg.com:443 + codeload.github.com:443 github.com:443 nightly.yarnpkg.com:443 nodejs.org:443 objects.githubusercontent.com:443 + raw.githubusercontent.com:443 + github-releases.githubusercontent.com:443 registry.yarnpkg.com:443 registry.npmjs.org:443 - 54.185.253.63:443
92-101: Foundry job allowlist: add GitHub API/download hosts; avoid brittle IP.Release downloads often hit github-releases and raw/codeload; include them; drop the IP.
allowed-endpoints: > binaries.soliditylang.org:443 + api.github.com:443 classic.yarnpkg.com:443 + codeload.github.com:443 github.com:443 nightly.yarnpkg.com:443 nodejs.org:443 objects.githubusercontent.com:443 + raw.githubusercontent.com:443 + github-releases.githubusercontent.com:443 registry.yarnpkg.com:443 registry.npmjs.org:443 - 54.185.253.63:443
🧹 Nitpick comments (3)
.github/workflows/contracts-testing.yml (3)
61-72: Revisit dependency caching for Yarn 4.With Yarn 4, caching node_modules may be ineffective; prefer caching .yarn/cache or rely on setup-node’s yarn cache where applicable.
Example swap:
- path: | - ~/.npm - **/node_modules + path: | + .yarn/cache
73-79: Make installs deterministic.Use a single root install with lockfile enforcement to match Yarn 4 best practices.
- - name: Install contracts dependencies - run: yarn workspace @kleros/kleros-v2-contracts install + - name: Install dependencies (immutable) + run: yarn install --immutable
133-141: Foundry install: fix trailing spaces; consider checksum/pinned action.
- Trailing spaces on Line 135 break lint rules.
- Optional: verify the tarball via SHA256 and/or use the pinned foundry-toolchain action for simpler installs.
- cd contracts + cd contractsAlternative install (optional):
- - name: Install Foundry - run: | - cd contracts - wget https://github.com/foundry-rs/foundry/releases/download/v1.3.4/foundry_v1.3.4_linux_amd64.tar.gz - tar -xzf foundry_v1.3.4_linux_amd64.tar.gz - ls -la - ./forge --version + - name: Install Foundry + uses: foundry-rs/foundry-toolchain@82dee4ba654bd2146511f85f0d013af94670c4de # v1.4.0 + with: + version: nightly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/contracts-testing.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/contracts-testing.yml
[error] 135-135: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
jaybuidl
force-pushed
the
feat/shutter-recovery-hash
branch
from
d44cb39 to
fb6ca6a
Compare
|
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/contracts-testing.yml (3)
92-101: Future-proof the commented Foundry job allowlist.When re-enabling, you’ll need the same GitHub endpoints.
- # allowed-endpoints: > - # binaries.soliditylang.org:443 - # classic.yarnpkg.com:443 - # github.com:443 - # nightly.yarnpkg.com:443 - # nodejs.org:443 - # objects.githubusercontent.com:443 - # registry.yarnpkg.com:443 - # registry.npmjs.org:443 - # 54.185.253.63:443 + # allowed-endpoints: > + # binaries.soliditylang.org:443 + # api.github.com:443 + # classic.yarnpkg.com:443 + # codeload.github.com:443 + # github.com:443 + # nightly.yarnpkg.com:443 + # nodejs.org:443 + # objects.githubusercontent.com:443 + # raw.githubusercontent.com:443 + # registry.yarnpkg.com:443 + # registry.npmjs.org:443 + # 54.185.253.63:443
19-21: Grant actions: write so actions/cache can save.Without this, caches won't persist. Add at workflow (or job) scope.
-permissions: # added using https://github.com/step-security/secure-workflows - contents: read +permissions: # added using https://github.com/step-security/secure-workflows + contents: read + actions: write
34-43: Allowlist core GitHub endpoints or the run will intermittently fail.Submodule checkout, Actions, and caches hit these domains.
allowed-endpoints: > binaries.soliditylang.org:443 + api.github.com:443 classic.yarnpkg.com:443 + codeload.github.com:443 github.com:443 nightly.yarnpkg.com:443 nodejs.org:443 objects.githubusercontent.com:443 + raw.githubusercontent.com:443 registry.yarnpkg.com:443 registry.npmjs.org:443 54.185.253.63:443
🧹 Nitpick comments (4)
.github/workflows/contracts-testing.yml (4)
49-54: Avoid double-setting Yarn version.Either prepare via corepack or use yarn set version; both is redundant.
- name: Set up corepack (for yarn) run: | corepack enable corepack prepare yarn@4.9.2 --activate - yarn set version 4.9.2
55-60: Make setup-node cache effective for Yarn by pinning dependency path.Optional: helps key correctness with Yarn.
- name: Setup Node.js environment uses: actions/setup-node@1d0ff469b7ec7b3cb9d8673fde0c81c44821de2a # v4.2.0 with: node-version: 20.x cache: yarn + cache-dependency-path: yarn.lock
76-79: Optional: Run tests via workspace from repo root for consistent Yarn context.- - name: Run Hardhat tests - run: yarn test - working-directory: contracts + - name: Run Hardhat tests + run: yarn workspace @kleros/kleros-v2-contracts test
29-43: Add a job timeout and (optional) concurrency control.Prevents hung jobs and cancels superseded runs on the same ref.
hardhat-tests: - runs-on: ubuntu-latest + runs-on: ubuntu-latest + timeout-minutes: 30 + concurrency: + group: contracts-tests-${{ github.ref }} + cancel-in-progress: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/contracts-testing.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
🔇 Additional comments (2)
.github/workflows/contracts-testing.yml (2)
45-48: Checkout config looks good.
61-72: Node-modules linker in use—keep caching node_modules
All.yarnrc.ymlfiles (web/.yarnrc.yml, root .yarnrc.yml, contracts/.yarnrc.yml) specifynodeLinker: node-modules, so caching**/node_modulesremains effective. Caching.yarn/cacheand.yarn/install-state.gzis only needed for offline installs or faster tarball retrieval.Likely an incorrect or invalid review comment.
✅ Actions performedComments resolved and changes approved. |
Closes #2016
_recoveryCommitin the eventCommitCastShutterrecoveryCommitmentsinDisputeKitShutterwhich collides withDisputeKitBase.wNativeadded recently.These changes require a full redeploy, not an upgrade.
PR-Codex overview
This PR primarily focuses on enhancing the
DisputeKitClassicBaseand its related contracts by improving the handling of vote commitments, adding recovery mechanisms, and optimizing the Solidity configurations for better performance.Detailed summary
evmVersion: "cancun"tohardhat.config.tsandfoundry.toml.optimizer_runsinfoundry.tomlto10000.DisputeKitClassicBase:_getExpectedVoteHashfunction.castCommitShutterandcastVoteShutterfunctions to handle recovery commitments.hashVoteto differentiate between juror and non-juror calls.DisputeKitShutterto cover new functionality and edge cases.Summary by CodeRabbit
New Features
Bug Fixes / Refactor
Tests
Chores